Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update vendor dependencies #221

Merged
merged 5 commits into from
Apr 14, 2020
Merged

Update vendor dependencies #221

merged 5 commits into from
Apr 14, 2020

Conversation

humblec
Copy link
Contributor

@humblec humblec commented Apr 2, 2020

Signed-off-by: Humble Chirammal [email protected]

Updated client-go to v0.18

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 2, 2020
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 2, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @humblec. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 2, 2020
@humblec humblec changed the title update kube dependency to v1.18.0 update vendor dependencies Apr 2, 2020
@humblec
Copy link
Contributor Author

humblec commented Apr 2, 2020

/release-note-none
/ok-to-test

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 2, 2020
@k8s-ci-robot
Copy link
Contributor

@humblec: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/release-note-none
/ok-to-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@humblec humblec changed the title update vendor dependencies [WIP] update vendor dependencies Apr 2, 2020
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 2, 2020
@jsafrane
Copy link
Contributor

jsafrane commented Apr 2, 2020

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 2, 2020
go.mod Outdated
github.com/imdario/mergo v0.3.7 // indirect
github.com/golang/mock v1.4.3
github.com/golang/protobuf v1.3.5
github.com/imdario/mergo v0.3.9 // indirect
github.com/kubernetes-csi/csi-lib-utils v0.7.0
github.com/kubernetes-csi/csi-test v2.0.0+incompatible
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also update to csi-test v3.x?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done..thx.

Signed-off-by: Humble Chirammal <[email protected]>
Signed-off-by: Humble Chirammal <[email protected]>
@humblec
Copy link
Contributor Author

humblec commented Apr 3, 2020

@jsafrane @pohly looks like CI is passing now, I am removing WIP . However I have added ctx and other params to API call to satisfy the new func signature due to the dependency update. Please review ..

@humblec humblec changed the title [WIP] update vendor dependencies Update vendor dependencies Apr 3, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 3, 2020
Copy link
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but before we merge I'd like to hear from @msau42 or @xing-yang how we want to handle the transition to client-go 1.18: we have some shared libraries (https://github.com/kubernetes-csi/csi-lib-utils, https://github.com/kubernetes-sigs/sig-storage-lib-external-provisioner) which still use the older client-go API. Shall we update all repos after the next release?

@msau42
Copy link
Collaborator

msau42 commented Apr 6, 2020

Yes, let's update all our repos to 1.18 client-go

@humblec
Copy link
Contributor Author

humblec commented Apr 6, 2020

kubernetes-sigs/sig-storage-lib-external-provisioner#73 cover s-s-l-external provisioner.

"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/kubernetes"
"k8s.io/klog"
)

var ctx context.Context = context.Background()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not use global variables. Use context.TODO() in all places where you use this.
Or even better, find a good API server timeout and use that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, let me start with context.TODO() local variable. Will do a follow up PR for configurable timeout . thanks !

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsafrane done.. changed to context.TODO() . .. @msau42 @pohly please review!

@humblec
Copy link
Contributor Author

humblec commented Apr 11, 2020

/assign @jsafrane
/assign @msau42
/assign @pohly

@msau42
Copy link
Collaborator

msau42 commented Apr 13, 2020

/lgtm
/approve

This may be worth adding a release note since the client-go update was signficant. Please add one and then unhold
/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Apr 13, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: humblec, msau42

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 13, 2020
@k8s-ci-robot
Copy link
Contributor

@humblec: the /release-note and /release-note-action-required commands have been deprecated.
Please edit the release-note block in the PR body text to include the release note. If the release note requires additional action include the string action required in the release note. For example:

```release-note
Some release note with action required.
```

In response to this:

/release-note

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@humblec
Copy link
Contributor Author

humblec commented Apr 14, 2020

@msau42 @pohly @jsafrane added release note . Please verify and unhold :).. thanks!

@jsafrane
Copy link
Contributor

The release note is for end users (admins), who do not care about internals of golang API calls. IMO something like "Updated client-go to v0.18" would be sufficient.

@humblec
Copy link
Contributor Author

humblec commented Apr 14, 2020

The release note is for end users (admins), who do not care about internals of golang API calls. IMO something like "Updated client-go to v0.18" would be sufficient.

@sure.. done.. !

@jsafrane
Copy link
Contributor

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 14, 2020
@jsafrane
Copy link
Contributor

Fixing failed travis by closing and reopening...

@jsafrane jsafrane closed this Apr 14, 2020
@jsafrane jsafrane reopened this Apr 14, 2020
@humblec
Copy link
Contributor Author

humblec commented Apr 14, 2020

I was wondering why this was stuck.. thanks @jsafrane 👍

@msau42
Copy link
Collaborator

msau42 commented Apr 14, 2020

travis is still stuck :( going to force merge. everything else is passing fine.

@msau42 msau42 merged commit 37007dc into kubernetes-csi:master Apr 14, 2020
@humblec
Copy link
Contributor Author

humblec commented Apr 14, 2020

Thanks @jsafrane and @msau42 !!

sunnylovestiramisu added a commit to sunnylovestiramisu/external-attacher that referenced this pull request Apr 12, 2023
6613c398 Merge pull request kubernetes-csi#223 from sunnylovestiramisu/update
0e7ae993 Update k8s image repo url
77e47cce Merge pull request kubernetes-csi#222 from xinydev/fix-dep-version
155854b0 Fix dep version mismatch
8f839056 Merge pull request kubernetes-csi#221 from sunnylovestiramisu/go-update
1d3f94dd Update go version to 1.20 to match k/k v1.27
e322ce5e Merge pull request kubernetes-csi#220 from andyzhangx/fix-golint-error
b74a5120 test: fix golint error
aa61bfd0 Merge pull request kubernetes-csi#218 from xing-yang/update_csi_driver
7563d196 Update CSI_PROW_DRIVER_VERSION to v1.11.0
a2171bef Merge pull request kubernetes-csi#216 from msau42/process
cb987826 Merge pull request kubernetes-csi#217 from msau42/owners
a11216e4 add new reviewers and remove inactive reviewers
dd986754 Add step for checking builds
b66c0824 Merge pull request kubernetes-csi#214 from pohly/junit-fixes
b9b6763b filter-junit.go: fix loss of testcases when parsing Ginkgo v2 JUnit
d4277839 filter-junit.go: preserve system error log
38e11468 prow.sh: publish individual JUnit files as separate artifacts

git-subtree-dir: release-tools
git-subtree-split: 6613c3980d1e418bebb7bc49d64c977cfff85671
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants